AsyncAPI 3.x: the driver contract for publishing a message - #1738
LautaroPetaccio wants to merge 7 commits into
Conversation
755b0a4 to
513d76a
Compare
513d76a to
45344be
Compare
45344be to
cbaae4c
Compare
cbaae4c to
cbea234
Compare
| /** | ||
| * Headers to publish alongside the body, for a transport that has them. | ||
| */ | ||
| public Map<String, String> headers = new LinkedHashMap<>(); |
There was a problem hiding this comment.
add a description of what the keys and values are in maps. This programming discipline is specified in docs/for_developers.md
| */ | ||
| public Long replyTimeoutMs; | ||
|
|
||
| public static final String CORRELATION_IN_HEADER = "HEADER"; |
There was a problem hiding this comment.
all constants should be at the beginning of the class
| /** | ||
| * The index of the action this answers, echoing what was asked. | ||
| */ | ||
| public Integer index; |
There was a problem hiding this comment.
It can't. It basically holds the same value as ActionDto.index, which newAction has already set by then, so a null never reaches this line. It is Integer to match ActionDto.index and ActionResponseDto.index.
EMController almost never assigns a DTO through constructors and most of the DTOs are plain classes without validations.
WDYT I should do in this case? Should I create this DTO to have a constructor with the integer with a validation?
There was a problem hiding this comment.
I think you should use int instead of Integer if this field is never null.
There was a problem hiding this comment.
I've converted it to int.
| * so this is a tuning parameter with no equivalent in a synchronous protocol. It is set | ||
| * generously and reported with the result. | ||
| */ | ||
| public Long replyTimeoutMs; |
There was a problem hiding this comment.
Yes, when no reply is expected (replyAddress null), there is nothing to wait for. I've written it in the javadoc.
cbea234 to
7750cb6
Compare
b59d655 to
0596618
Compare
61ca390 to
018bf2b
Compare
018bf2b to
1a05716
Compare
19da395 to
564e9b8
Compare
09199ad to
b2063e0
Compare
| /** | ||
| * The index of the action this answers, echoing what was asked. | ||
| */ | ||
| public Integer index; |
There was a problem hiding this comment.
I think you should use int instead of Integer if this field is never null.
b2063e0 to
e24137a
Compare
Everything so far is in core and reads a document. This is the other side: what a driver must provide so that the core can drive an AsyncAPI service without knowing anything about brokers. AsyncApiProblem declares that the SUT is driven by messages. It carries only the document -- a location to fetch it from, or the text itself. The connection to the broker stays in the driver and never crosses, which is what keeps the core free of any broker library. Note it may be declared with the document inline, which REST has no real need for. A REST service usually serves its own contract over HTTP; a service that speaks only Kafka has no endpoint to serve anything from, so its document is far more likely to be a file shipped beside it. SutController.executeAsyncApiAction is where a driver publishes one message and, when a reply is expected, waits for the one that answers it. It is the counterpart of executeAction for RPC. It has a default that throws rather than being abstract, so that adding it does not break every existing driver; the message says what to override. Only publish and await are protocol-specific, and they never leave the driver. The driver is asked to report what happened, not to judge it: AsyncApiReplyDto distinguishes published-with-no-reply-expected, a reply that arrived, silence within the window, and a failure to publish at all. The last of those is a broken setup rather than a finding about the service, which is why it is kept apart from silence. Deciding what an outcome means is the core's job, so that it means the same thing whatever the transport. Also carried in the reply is whether the correlation id came back. Whether correlation works cannot be read off a contract, since echoing the id is the service's own behaviour, so it is established by watching for it. Wiring: a field on SutInfoDto, a field on ActionDto, and two branches in EMController.
Review feedback: CORRELATION_IN_HEADER and CORRELATION_IN_PAYLOAD closed the class, after every instance field. docs/for_developers.md puts constants first.
Review feedback, and docs/for_developers.md: a Map field says what its key and value are. Both header maps go from header name to its text.
A null list failed inside addAll anyway; the check now sits where the method starts, named, as docs/for_developers.md asks of public methods.
Review feedback: replyTimeoutMs is null when no reply is expected, and waitedMs when the driver did not wait at all, which the javadoc left unsaid.
Review feedback: it echoes ActionDto.index, which newAction has already dereferenced by the time the reply is built, so an int says what the Integer could only promise.
e24137a to
0e2c12e
Compare
| /** | ||
| * The index of the action this answers, echoing what was asked. | ||
| */ | ||
| public int index; |
There was a problem hiding this comment.
DTOs should NEVER use primitive values, as missing values would be silently converted to defaults
There was a problem hiding this comment.
Hi @arcuri82 , this was my bad since I asked @LautaroPetaccio to explicitly change all non-nullable fields to their primitive types in Lautaro's commit. Thanks for clarifying this behaviour explicitly for DTOs.
| * Whether the message reached the broker. False means the driver could not publish, and | ||
| * {@link #errorMessage} says why. | ||
| */ | ||
| public boolean published; |
There was a problem hiding this comment.
no primitive values in DTOs
| /** | ||
| * Whether a reply arrived and was recognised as answering this message. | ||
| */ | ||
| public boolean replyReceived; |
There was a problem hiding this comment.
no primitive values in DTOs
| * Whether the driver waited for a reply at all. False for a fire-and-forget operation, so | ||
| * that the absence of a reply is not mistaken for silence in answer to a promise. | ||
| */ | ||
| public boolean replyExpected; |
There was a problem hiding this comment.
no primitive values in DTOs
| * recorded rather than treated as a fault, since from outside there is no telling a defect | ||
| * from a service that correlates by some business key instead. | ||
| */ | ||
| public boolean correlationMatched; |
There was a problem hiding this comment.
no primitive values in DTOs
|
|
||
| private AsyncApiProblem(String schemaLocation, String schemaText) { | ||
|
|
||
| if ((schemaLocation == null) == (schemaText == null)) { |
There was a problem hiding this comment.
bit convoluted check. for readability, use explicit check, eg:
if ((schemaLocation == null && schemaText == null) || (schemaLocation != null && schemaText != null))
A primitive turns a field the driver never set into a default, with no way to tell the two apart. index, published, replyReceived, replyExpected and correlationMatched are boxed, and each says what null means. The distinction earns its keep on correlationMatched: a driver that does not track correlation at all said false before, which reads as the service having failed to echo the id back. That is a claim about the SUT, and it was not one the driver ever made. The core reads these, so it decides what an absent value means rather than inheriting a default; that half comes with the fitness. Also: the check for exactly one of a location and a text is written out, rather than comparing two null-checks for equality.
Ninth in the AsyncAPI stack, on top of #7. This one is entirely in
client-java— no core changes, so it reviews as controller-side plumbing on its own.Everything so far reads a document. This is the other side: what a driver must provide so the core can drive an AsyncAPI service without knowing anything about brokers.
Why a driver at all, even for black-box
Unlike REST and GraphQL, there is no universal wire to point at — Kafka, AMQP, MQTT and WebSocket share nothing at the API level. So something has to hold a client and move the bytes, and that something is the driver. This is the one way AsyncAPI is unlike every other black-box mode EvoMaster ships, and it is the same reason RPC needs a driver.
AsyncApiProblemCarries only the document — a location to fetch it from, or the text itself. The connection to the broker stays in the driver and never crosses, which is what keeps core free of any broker dependency.
The inline-text option is worth noting, since REST has little need for it: a REST service usually serves its own contract over HTTP, whereas a service that speaks only Kafka has no endpoint to serve anything from. Its document is far more likely to be a file shipped beside it.
SutController.executeAsyncApiActionWhere a driver publishes one message and, when a reply is expected, waits for the one that answers it — the counterpart of
executeActionfor RPC.It has a default that throws rather than being abstract, so adding it does not break any existing driver; the message names what to override. Only publish and await are protocol-specific, and they never leave the driver.
The driver reports, it does not judge
AsyncApiReplyDtodistinguishes four outcomes, because they mean different things and only one is a fault:Deciding what an outcome means is the core's job, so that it means the same thing whatever the transport.
The reply also carries whether the correlation id came back. Whether correlation works cannot be read off a contract — echoing the id is the service's own behaviour — so it is established by watching for it rather than assumed.
Wiring
A field on
SutInfoDto, a field onActionDto, and two branches inEMController.Testing
Six tests in
AsyncApiProblemTest, including one asserting that a driver pointed at an AsyncAPI service without implementing the hook fails with a message telling it what to override.client-java/controllerruns 740 tests, 0 failures, 34 errors — identical with and without this change. Those 34 are testcontainers suites failing because Docker is not reachable in my environment; I verified the same counts on a pristine tree before claiming that.